feat: merge models discovery into the model noun + deprecation-alias helper (BE-2999)#521
feat: merge models discovery into the model noun + deprecation-alias helper (BE-2999)#521mattmillerai wants to merge 3 commits into
models discovery into the model noun + deprecation-alias helper (BE-2999)#521Conversation
…ias helper (BE-2999) Collapse the confusing `model` (singular, local-filesystem ops) vs `models` (plural, backend/cloud discovery) split into a single `model` noun group. - New shared helper comfy_cli/deprecation.py: add_deprecated_alias() mounts a hidden alias whose group help is prefixed [DEPRECATED — use <new>] and which prints one yellow warning to stderr on invocation (never silenced). Sibling noun consolidations reuse it. - Surface the four discovery leaves (list-folders/list-folder/search/show) under `comfy model` by borrowing search.app's command registrations (same CommandInfo objects — no logic duplication). - Keep `comfy models …` working as a hidden, deprecated alias via the helper. - Distinguish help text so `list` (local on disk) vs `list-folder` (backend/ cloud) no longer reads as a typo. - Register the canonical `comfy model …` paths in COMMAND_SCHEMAS (keeping the plural, since envelopes still carry the `models …` command string) and update the bundled SKILL.md examples to the canonical spelling. Non-breaking: every old `comfy models …` invocation still works.
dosubot
Bot
added
the
size:L
mattmillerai
added
agent-coded
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
Based on my investigation of the actual code, I can now adjudicate the panel findings.
Key verifications:
- `comfy_cli/command/models/search.py`'s `app` has **no `@app.callback()` and no sub-groups** (`add_typer`) — only four `@app.command` leaves. So gemini's findings 3/4/5 (dropped `registered_groups`/`registered_callback`) describe **no current bug**; they're a latent robustness trap for the documented-reusable helper. These three are near-duplicates of one root cause and merge into one.
- `
Re-trigger by removing and re-adding the cursor-review label.
click >=8.2 removed the CliRunner(mix_stderr=...) kwarg (stdout/stderr are always captured separately now), which broke the pytest CI job that installs deps fresh. Detect the kwarg via signature inspection: pass mix_stderr=False on click <8.2, plain CliRunner() on >=8.2. result.stderr stays split on both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… helper (BE-2999) add_deprecated_alias is documented as a reusable helper for the sibling noun consolidations, but it only copied registered_commands — silently dropping any nested sub-groups (add_typer) and the source app's own group callback. Harmless for the current `models`→`model` mount (a flat, callback-less command tree) but a latent robustness trap for future reuse. Now also extend registered_groups and, when the source declares a callback, compose it with the deprecation warning (functools.wraps preserves its option signature) instead of overriding it. Adds regression tests for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the one substantive point surfaced by the Cursor panel (its judge crashed with a Harmless for the current The failing Windows |
|
Re-ran the failed `Windows Specific Commands` job; it failed again in the dependency-install step (not the test run): ``` Confirmed repo-wide, pre-existing infra flake — the same job is currently red on every open branch (be-3000, be-2994, be-2982, be-2978, be-3001, be-2975, …), so it is unrelated to this diff (model discovery + deprecation alias, no dependency changes). Fixing it belongs in the CI workflow, not this PR. The |
1 participant
ELI-5
comfyhad two confusingly-split command groups:comfy model(singular — yourlocal model files on disk: download / remove / list) and
comfy models(plural —asking a backend/cloud server what models exist: list-folders / list-folder /
search / show). Two nouns for "models" is easy to fumble, and
model listvsmodels list-folderread like typos of each other.This merges everything under the single
modelnoun. Nowcomfy model search,comfy model show,comfy model list-folders, andcomfy model list-folderallwork. The old
comfy models …spelling still works exactly as before, but it'shidden from
--helpand prints a one-line yellow "deprecated; usecomfy model …"warning to stderr. Nothing breaks.
What changed
comfy_cli/deprecation.py—add_deprecated_alias(parent, source_app, old_name, new_name)mounts a hidden alias of a Typer group whosegroup help is prefixed
[DEPRECATED — use <new>]and which emits a single[yellow]warning to stderr on invocation (never silenced — the warning isthe point, and stderr keeps the JSON-envelope-on-stdout contract intact). The
sibling noun consolidations reuse this helper.
modelnow owns the discovery leaves — the four leaves are surfaced undercomfy modelby borrowingsearch.app's command registrations (the sameCommandInfoobjects — the implementations are reused, not duplicated). Thismirrors the existing
skill/skillsalias reuse pattern.modelsis a hidden, deprecated alias wired through the new helper; its fourleaves still work and print the warning.
comfy model listsays "local models on disk";list-folders/list-foldersay "backend/cloud", solistvslist-foldernolonger reads as a typo.
comfy model …paths inCOMMAND_SCHEMAS(keeping the plural too, since the emitted envelopecommandfield still carries
models …, so both spellings must resolve — same dual-regas
skill/skills). Updated the bundledSKILL.mdexamples to the canonicalspelling.
Non-breaking
Yes. Every old
comfy models …invocation still works via the alias, and the JSONenvelope
commandfield is unchanged (models list-folders, etc.) — no contractchange for JSON consumers.
Tests
tests/comfy_cli/command/models/test_model_alias.py: asserts the fourdiscovery leaves resolve under
comfy model; the local ops (download/remove/list)are undisplaced;
comfy models …still resolves but is hidden with the deprecationbanner; unit tests for the helper; and end-to-end that
comfy models list-foldersemits an ok envelope on stdout and the warning on stderr, while
comfy model list-foldersis warning-free.tests/comfy_cli/output/test_discovery.pyto require the canonicalcomfy model …schema registrations alongside the plural.ruff format --check+ruff checkclean on all touched files. (The 14 pre-existing repo-wide
UP038lint findingsare in untouched files — not introduced here.)
Judgment calls
commandfield asmodels …(notmodel …) to stay strictlynon-breaking for JSON consumers; that's why both spellings are registered in
COMMAND_SCHEMAS, exactly like the pre-existingskill/skillsalias.@tracking.track_command("models")on the reused leaves is left as-is (a cosmeticanalytics group label), out of scope for this rename.